Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nissan/carcontroller: add max steering angle safeguard for nissan #33020

Closed

Conversation

bongbui321
Copy link
Contributor

@bongbui321 bongbui321 commented Jul 19, 2024

Resolves: commaai/opendbc#1145

Unlikely this is going to happen in real scenario, but I think it's worth to have a safeguard. It seems that panda decodes by adding a 1310 offset to the angle

https://github.com/commaai/panda/blob/f6375848ca393a9483921665b6a2d131d7ec9b20/board/safety/safety_nissan.h#L107

@github-actions github-actions bot added car vehicle-specific nissan labels Jul 19, 2024
Copy link
Contributor

github-actions bot commented Jul 19, 2024

Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • the change is something we merge
    • include a route or your device' dongle ID if relevant

@bongbui321 bongbui321 changed the title Nissan/carcontroller: add_max_steering_angle_nissan Nissan/carcontroller: add max steering angle safeguard for nissan Jul 19, 2024
@sshane
Copy link
Contributor

sshane commented Jul 20, 2024

It could make sense to have a global angle limit for angle control cars that's well below the max of any car. Also would be nice if CANPacker gave a warning for out of range values instead of wrapping

Copy link
Contributor

@adeebshihadeh adeebshihadeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change itself looks fine, but let's get this fixed up in the CANPacker too:

  • CANPacker prints a warning when this happens
  • CANPacker can optionally assert on that warning, and we'll set that option in CI. similar to PYTHONWARNINGS=error or -Werror

@bongbui321 bongbui321 marked this pull request as draft July 25, 2024 17:31
@bongbui321
Copy link
Contributor Author

It could make sense to have a global angle limit for angle control cars that's well below the max of any car

This sounds nice, though I'm hesitant to put up PRs for this knowing very little on car stuff. maybe just get the max angle from the angle possible from angle-based cars?

@bongbui321 bongbui321 marked this pull request as ready for review July 27, 2024 19:17
@sshane
Copy link
Contributor

sshane commented Aug 20, 2024

We've moved the car interfacing code to our opendbc repository, which is now the new home for car ports and fingerprints. Please re-open your pull request against opendbc at your convenience by using this command below. This will transform all changes under selfdrive/car/ to opendbc/opendbc/car/. Make sure you have initialized submodules beforehand and have checked out the latest opendbc commit.

PR_NUMBER=33045
curl -L https://github.com/commaai/openpilot/pull/$PR_NUMBER.patch | sed -e 's/selfdrive\/car/opendbc_repo\/opendbc\/car/g' | git apply -v --reject

Simply replace the PR number with your own. Once done, add the files, fix any conflicts, and open a new PR. Alternatively, you may start a new PR from scratch if that is easier for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
car vehicle-specific nissan
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CANPacker for Nissan produces inconsistent desired_angle with Openpilot when it is large
3 participants